Skip to content

Fix Socket#reuse_port? if SO_REUSEPORT is not supported#6706

Merged
RX14 merged 2 commits intocrystal-lang:masterfrom
straight-shoota:jm/fix/socket-reuse-port
Sep 12, 2018
Merged

Fix Socket#reuse_port? if SO_REUSEPORT is not supported#6706
RX14 merged 2 commits intocrystal-lang:masterfrom
straight-shoota:jm/fix/socket-reuse-port

Conversation

@straight-shoota
Copy link
Copy Markdown
Member

@straight-shoota straight-shoota commented Sep 11, 2018

Currently, if SO_REUSEPORT is not supported, LibC.getsockopt returns -1. When calling Socket#reuse_port? this causes an exception.

This PR fixes that to return false if errno is ENOPROTOOPT (meaning that SO_REUSEPORT is not supported).

Sija
Sija reviewed Sep 11, 2018
View reviewed changes
src/socket.cr Outdated

def reuse_port?
getsockopt_bool LibC::SO_REUSEPORT
0 != getsockopt(LibC::SO_REUSEPORT, 0) do |errno|
Copy link
Copy Markdown
Contributor

@Sija Sija Sep 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, that looks sooo weird... Please, assign the return value to a variable and compare with that:

ret = getsockopt(LibC::SO_REUSEPORT, 0) do |errno|
  # ...
end
ret != 0

@ysbaddaden
Copy link
Copy Markdown
Collaborator

ysbaddaden commented Sep 12, 2018

I'm not sure. reuse_port? is meant to say whether SO_REUSEPORT is set, not swallow the fact that it's unsupported.

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 12, 2018 •
edited
Loading

My thinking is: If SO_REUSEPORT is not supported, it can't be set. So the method should return false.

socket.reuse_port = true also works if SO_REUSEPORT is not supported. If anything should raise, it's the setter. Or both. Or none. But it makes no sense if only the getter raises.

I prefer either both or only the setter should raise. reuse_port? should just honestly answer if the socket has the port flagged for reuse which is false if SO_REUSEPORT is not supported at all.

@ysbaddaden
Copy link
Copy Markdown
Collaborator

ysbaddaden commented Sep 12, 2018

My thinking is "can I check reuse_port? before setting it?" If so, I could be wrongly led to believe it worked since the setter won't set errno, which is probably an issue by itself.

So yeah, maybe the check should just return false, but trying to set the value must raise if unsupported.

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 12, 2018 •
edited
Loading

I've done some research and from what I've read, it seams that getsockopt should actually fail with ENOPROTOOPT. So #setsockopt would raise in that case.

But, at least on WSL, setsockopt doesn't even complain about setting SO_REUSEPORT, only getsockopt.

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 12, 2018

There are however reports about setsockopt failing on WSL: Kitura/BlueSocket#81 microsoft/WSL#1419
But I can't reproduce it in Crystal on WSL.

@straight-shoota straight-shoota mentioned this pull request Sep 12, 2018
Open
@ysbaddaden
Copy link
Copy Markdown
Collaborator

ysbaddaden commented Sep 12, 2018

Let Microsoft seal with WSL bugs. setsockopt should fail which is fine.

ysbaddaden
ysbaddaden approved these changes Sep 12, 2018
View reviewed changes
src/socket.cr
def reuse_port?
getsockopt_bool LibC::SO_REUSEPORT
ret = getsockopt(LibC::SO_REUSEPORT, 0) do |errno|
# If SO_REUSEPORT is not supported, the return value should be `false`
Copy link
Copy Markdown
Collaborator

@ysbaddaden ysbaddaden Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the comment: code is explicit enough.

@RX14 RX14 added kind:feature topic:stdlib breaking-change May have breaking effect in some edge cases. Mostly negligible, but still noteworthy. kind:bug A bug in the code. Does not apply to documentation, specs, etc. and removed breaking-change May have breaking effect in some edge cases. Mostly negligible, but still noteworthy. kind:feature labels Sep 12, 2018
@RX14 RX14 added this to the 0.27.0 milestone Sep 12, 2018
@RX14 RX14 merged commit c6ee7bc into crystal-lang:master Sep 12, 2018
@straight-shoota straight-shoota deleted the jm/fix/socket-reuse-port branch September 12, 2018 16:40
@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 12, 2018

@ysbaddaden

setsockopt should fail which is fine.

The point is, it doesn't fail on WSL :(

@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 12, 2018

@straight-shoota

Let Microsoft seal with WSL bugs

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 12, 2018

Yeah, sure. That's why I posted a comment in the WSL issue. It's just that Crystal's #reuse_port= method also behaves incorrect. It's not a big deal of course.

ezrast pushed a commit to ezrast/crystal that referenced this pull request Oct 2, 2018
@straight-shoota
Fix Socket#reuse_port? if SO_REUSEPORT is not supported (crystal-lang…
b801262 
…#6706)

* Fix Socket#reuse_port? if SO_REUSEPORT is not supported

* fixup! Fix Socket#reuse_port? if SO_REUSEPORT is not supported
@straight-shoota straight-shoota mentioned this pull request Oct 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ysbaddaden ysbaddaden ysbaddaden approved these changes

@jhass jhass jhass approved these changes

@RX14 RX14 RX14 approved these changes

+1 more reviewer

@Sija Sija Sija left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib

Projects

None yet

Milestone

0.27.0

Development

Successfully merging this pull request may close these issues.

5 participants

@straight-shoota @ysbaddaden @RX14 @Sija @jhass